Skip to content

Conversation

@Abhijay007
Copy link
Collaborator

ref : #6557 (comment), #6557 (comment)

PR Description

updated elevenLabs API module and remove UX

Type of Change

  • Refactor / Code quality

AI Assistance

  • This PR was created or reviewed with AI assistance

Testing

Tested In Desktop UI

Screenshots/Demos (for UX changes)

updateUXAPI

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors the ElevenLabs API key settings UI in the desktop app by simplifying state management and removing some loading/validation UX.

Changes:

  • Simplifies initial key detection to a single useEffect check.
  • Switches save/remove flows to optimistic local state updates (no reload-based recheck).
  • Adjusts the UI to show a dedicated “Remove API Key” action outside edit mode and removes inline validation/error messaging.

@Abhijay007 Abhijay007 changed the title refactor: updated elevenLabs API module and remove UX refactor: updated elevenLabs API module and remove button UX Jan 28, 2026
Copy link
Collaborator

@lifeizhou-ap lifeizhou-ap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @Abhijay007 for this improvement!

@Abhijay007
Copy link
Collaborator Author

@DOsinga, could you please take a look at this as well? I believe it’s ready to be merged. I’m asking since you had initially mentioned the refactor in the other PR

@Abhijay007 Abhijay007 force-pushed the refactor/elevenLabsModule branch from b6ae3ab to aa0da6c Compare February 2, 2026 18:10
@Abhijay007
Copy link
Collaborator Author

Abhijay007 commented Feb 2, 2026

Hi @DOsinga , @lifeizhou-ap I updated this PR as during the recent refactor in this PR : #6844, it broke ElevenLabs by using ApiClient wrapper instead of direct HTTP client so I reverted that and updated remove button UX, which was requested before

@DOsinga
Copy link
Collaborator

DOsinga commented Feb 2, 2026

I'm sorry I refactored the entire way we do this over the weekend - the config now comes from the server, you'll have a terrible merge conflict on your hands

@Abhijay007
Copy link
Collaborator Author

Abhijay007 commented Feb 2, 2026

I'm sorry I refactored the entire way we do this over the weekend - the config now comes from the server, you'll have a terrible merge conflict on your hands

no problem I updated it , you can review it now :) @DOsinga

Copilot AI review requested due to automatic review settings February 2, 2026 18:15
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

crates/goose-server/src/routes/dictation.rs:270

  • The ElevenLabs multipart part creation discards the underlying reqwest error (map_err(|_| ...)), which makes failures harder to diagnose; preserve the original error message (as is done in transcribe_openai) when constructing the internal ErrorResponse.
    let part = reqwest::multipart::Part::bytes(audio_bytes)
        .file_name(format!("audio.{}", extension))
        .mime_str(mime_type)
        .map_err(|_| ErrorResponse::internal("Failed to create multipart"))?;

setKeyValidationError('API key is required');
return;
}
if (!trimmedKey) return;
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clicking “Save” with an empty/whitespace API key currently just returns with no UI feedback, which makes the button appear broken; disable the Save button until a non-empty key is entered or reintroduce a small inline validation message.

Suggested change
if (!trimmedKey) return;
if (!trimmedKey) {
window.alert('Please enter a non-empty API key before saving.');
return;
}

Copilot uses AI. Check for mistakes.
Comment on lines +276 to +279
let http_client = reqwest::Client::builder()
.timeout(REQUEST_TIMEOUT)
.build()
.map_err(|e| ErrorResponse::internal(format!("Failed to create HTTP client: {}", e)))?;
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates a new reqwest::Client for every ElevenLabs transcription request, which prevents connection pooling and adds unnecessary overhead; consider reusing a shared client (e.g., stored in AppState or a static Lazy client) configured with the same timeout.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@DOsinga DOsinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before we continue, can you make sure you merge in main? this looks different and sorry again for changing everything

Ok(data.text)
}

async fn transcribe_elevenlabs(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't match main, I removed all specific transcribing, there is now just a transcribe_dictation method that takes a DictationProvider to indicate how to transcribe

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

umm that's strange, let me look into it, I remember rebasing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well this is in main rn:

async fn transcribe_elevenlabs(
I am not sure if that came from some other PR: cc @DOsinga

@Abhijay007 Abhijay007 force-pushed the refactor/elevenLabsModule branch from 5e30bda to 12a3128 Compare February 2, 2026 18:32
@DOsinga
Copy link
Collaborator

DOsinga commented Feb 2, 2026

we have a merge queue now after some incidents, so it might take a tad longer to see the latest in main

@Abhijay007
Copy link
Collaborator Author

we have a merge queue now after some incidents, so it might take a tad longer to see the latest in main

Oh, okay so should we wait ? Like when I am taking pull, it is not reflecting for me

@DOsinga
Copy link
Collaborator

DOsinga commented Feb 2, 2026

hmm, what does your routers/dictation.rs ends on? mine has:

pub fn routes(state: Arc) -> Router {
Router::new()
.route("/dictation/transcribe", post(transcribe_dictation))
.route("/dictation/config", get(get_dictation_config))
.with_state(state)
}

@Abhijay007
Copy link
Collaborator Author

hmm, what does your routers/dictation.rs ends on? mine has:

pub fn routes(state: Arc) -> Router { Router::new() .route("/dictation/transcribe", post(transcribe_dictation)) .route("/dictation/config", get(get_dictation_config)) .with_state(state) }

same for me

@DOsinga
Copy link
Collaborator

DOsinga commented Feb 2, 2026

oh, I am sorry, yes, that is main. I do have another refactor though that changes even more coming up if you can wait on that? that generalizes things even more

@Abhijay007
Copy link
Collaborator Author

Abhijay007 commented Feb 2, 2026

oh, I am sorry, yes, that is main. I do have another refactor though that changes even more coming up if you can wait on that? that generalizes things even more

Yea sure will update the PR once those changes get merged

@DOsinga
Copy link
Collaborator

DOsinga commented Feb 3, 2026

merged

@Abhijay007
Copy link
Collaborator Author

Sure will update it today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants